-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[libc++] Optimize multi{map,set}::insert(InputIterator, InputIterator) #152691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
2f03769
to
8bed2d3
Compare
@@ -970,6 +970,36 @@ public: | |||
__emplace_hint_multi(__p, std::move(__value)); | |||
} | |||
|
|||
template <class _InIter, class _Sent> | |||
_LIBCPP_HIDE_FROM_ABI void __insert_range_multi(_InIter __first, _Sent __last) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's document what this function does and in particular what optimizations it performs (i.e. it is optimized for already-sorted inputs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What sort of documentation do you want? I'm not sure there is much to say beyond "inserts the range [first, last) into *this", which I think is rather obvious.
@@ -970,6 +970,36 @@ public: | |||
__emplace_hint_multi(__p, std::move(__value)); | |||
} | |||
|
|||
template <class _InIter, class _Sent> | |||
_LIBCPP_HIDE_FROM_ABI void __insert_range_multi(_InIter __first, _Sent __last) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also suggest taking a tag here and handling the (very similar) unique/multi cases uniformly with a if constexpr
. Then we can report improvements for map
and set
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that adding a unique implementation will be a lot more complex, since we have a lot more complex handling in __emplace_unique
to avoid unnecessary allocations if the node already exists. I think it would be better to refactor __emplace_unique
to make that simpler first and then tackle __insert_range_unique
(or a merged version) separately.
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) Changes
Full diff: https://github.com/llvm/llvm-project/pull/152691.diff 3 Files Affected:
diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index 6dadd0915c984..5baadca719ac4 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -970,6 +970,36 @@ public:
__emplace_hint_multi(__p, std::move(__value));
}
+ template <class _InIter, class _Sent>
+ _LIBCPP_HIDE_FROM_ABI void __insert_range_multi(_InIter __first, _Sent __last) {
+ if (__first == __last)
+ return;
+
+ if (__root() == nullptr) { // Make sure we always have a root node
+ __node_holder __node = __construct_node(*__first);
+ __insert_node_at(__end_node(), __end_node()->__left_, static_cast<__node_base_pointer>(__node.release()));
+ ++__first;
+ }
+
+ auto __max_node = static_cast<__node_pointer>(std::__tree_max(static_cast<__node_base_pointer>(__root())));
+ __node_pointer __last_insertion = __root();
+
+ for (; __first != __last; ++__first) {
+ __node_holder __node = __construct_node(*__first);
+ if (!value_comp()(__node->__get_value(), __max_node->__get_value())) { // __node >= __max_val
+ __insert_node_at(static_cast<__end_node_pointer>(__max_node),
+ __max_node->__right_,
+ static_cast<__node_base_pointer>(__node.get()));
+ __max_node = __node.release();
+ } else {
+ __end_node_pointer __parent;
+ __node_base_pointer& __child = __find_leaf(++iterator(__last_insertion), __parent, __node->__value_);
+ __last_insertion = __node.get();
+ __insert_node_at(__parent, __child, static_cast<__node_base_pointer>(__node.release()));
+ }
+ }
+ }
+
_LIBCPP_HIDE_FROM_ABI pair<iterator, bool> __node_assign_unique(const value_type& __v, __node_pointer __dest);
_LIBCPP_HIDE_FROM_ABI iterator __node_insert_multi(__node_pointer __nd);
diff --git a/libcxx/include/map b/libcxx/include/map
index 9f98abef9afe0..a84b5592244fa 100644
--- a/libcxx/include/map
+++ b/libcxx/include/map
@@ -593,6 +593,7 @@ erase_if(multimap<Key, T, Compare, Allocator>& c, Predicate pred); // C++20
# include <__memory/unique_ptr.h>
# include <__memory_resource/polymorphic_allocator.h>
# include <__node_handle>
+# include <__ranges/access.h>
# include <__ranges/concepts.h>
# include <__ranges/container_compatible_range.h>
# include <__ranges/from_range.h>
diff --git a/libcxx/include/set b/libcxx/include/set
index c77345bc5dc1f..18488b56e7dd2 100644
--- a/libcxx/include/set
+++ b/libcxx/include/set
@@ -530,6 +530,7 @@ erase_if(multiset<Key, Compare, Allocator>& c, Predicate pred); // C++20
# include <__memory/allocator_traits.h>
# include <__memory_resource/polymorphic_allocator.h>
# include <__node_handle>
+# include <__ranges/access.h>
# include <__ranges/concepts.h>
# include <__ranges/container_compatible_range.h>
# include <__ranges/from_range.h>
@@ -1205,18 +1206,14 @@ public:
}
template <class _InputIterator>
- _LIBCPP_HIDE_FROM_ABI void insert(_InputIterator __f, _InputIterator __l) {
- for (const_iterator __e = cend(); __f != __l; ++__f)
- __tree_.__emplace_hint_multi(__e, *__f);
+ _LIBCPP_HIDE_FROM_ABI void insert(_InputIterator __first, _InputIterator __last) {
+ __tree_.__insert_range_multi(__first, __last);
}
# if _LIBCPP_STD_VER >= 23
template <_ContainerCompatibleRange<value_type> _Range>
_LIBCPP_HIDE_FROM_ABI void insert_range(_Range&& __range) {
- const_iterator __end = cend();
- for (auto&& __element : __range) {
- __tree_.__emplace_hint_multi(__end, std::forward<decltype(__element)>(__element));
- }
+ __tree_.__insert_range_multi(ranges::begin(__range), ranges::end(__range));
}
# endif
|
9b9dbda
to
0e5d4f1
Compare
0e5d4f1
to
691222a
Compare
Uh oh!
There was an error while loading. Please reload this page.